Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [#2] addIfAbsent must resolve module before comparison #3

Merged
merged 2 commits into from
Dec 7, 2019

Conversation

moarcaffeine
Copy link
Contributor

Current check is comparing names to resolved modules. Fix for issue #2.

Current check is comparing names to resolved modules.
@davidje13
Copy link
Owner

Interesting that I haven't seen the bug you describe myself (maybe it's OS-specific or only manifests for projects in paths with capital letters, etc.). This change looks fine, but 2 comments:

  1. Is includes still needed, or is straightforward equality checking sufficient?
  2. I assume require.resolve is somewhat expensive, so can you pull it out into a variable to avoid computing it for each check? (if it does its own internal caching then nevermind)

I can merge and push a new release tomorrow.

Use full string equality rather than partial matches, and resolve each path only once.
@davidje13 davidje13 merged commit 43e4a01 into davidje13:master Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants